Implement interaction groups test mode and cofficient combine rule #741
Conversation
Ughuuu
commented
Sep 22, 2024
Ughuuu
commented
- Continues Add InteractionGroups::test_or, increase to 64 bits #170
- Fixes Feature: Add support for custom friction and bounce CombineRules #622
|
Ok, added this mainly to stop using my fork and to try to use rapier default in godot-rapier. This way I hope to no longer need to maintain that and use this repo as it is. (if this gets merged) |
Ughuuu
force-pushed
the
Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2
branch
3 times, most recently
from
b05dc45
to
1d0db7e
Compare
update fix docs cargo format fix update feedback update Update CHANGELOG.md ignore line for error docs fix update
Ughuuu
force-pushed
the
Add-interaction-or,-increase-to-64-bit-and-add-more-rules-take-2
branch
from
bb2b387
to
0db7c72
Compare
|
Fixed the doc test from CI. |
| @@ -27,8 +29,10 @@ impl CoefficientCombineRule { | |||
|
|
|||
| match effective_rule { | |||
| 0 => (coeff1 + coeff2) / 2.0, | |||
| 1 => coeff1.min(coeff2), | |||
| 1 => coeff1.min(coeff2).abs(), | |||
There was a problem hiding this comment.
(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?
There was a problem hiding this comment.
Should it be max(0.0) ?
I think there is some use-case for this in godot but that would be worth clarifying by @Ughuuu.
(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?
I don’t think there is a particular reason for this.
| pub const fn test(self, rhs: Self) -> bool { | ||
| match self.test_mode { | ||
| InteractionTestMode::AND => self.test_and(rhs), | ||
| InteractionTestMode::OR => self.test_or(rhs), | ||
| } | ||
| } |
There was a problem hiding this comment.
This will create some hard to predict behavior if one of the interaction groups is set to AND while the other is set to OR. In that situation, whichever is self in this test will take precedence (and predicting which collider will be first here is not possible as it depends on some chains of events happening on the broad/narrow phase). I suggest setting an explicit precedence rule where InterecationTestMode::AND always has the priority:
| pub const fn test(self, rhs: Self) -> bool { | |
| match self.test_mode { | |
| InteractionTestMode::AND => self.test_and(rhs), | |
| InteractionTestMode::OR => self.test_or(rhs), | |
| } | |
| } | |
| pub const fn test(self, rhs: Self) -> bool { | |
| if self.test_mode == InteractionTestMode::And || rhs.test_mode == InteractionTestMode::And { | |
| self.test_and(rhs) | |
| } else { | |
| self.test_or(rhs) | |
| } |
This precedence rule should be explained in the InteractionTestMode and InteractionGroups docs.
| #[default] | ||
| AND, | ||
| /// Use [`InteractionGroups::test_or`]. | ||
| OR, |
There was a problem hiding this comment.
The case is not idiomatic:
| #[default] | |
| AND, | |
| /// Use [`InteractionGroups::test_or`]. | |
| OR, | |
| #[default] | |
| And, | |
| /// Use [`InteractionGroups::test_or`]. | |
| Or, |
There was a problem hiding this comment.
@Vrixyz Can you check why the non-idiomatic case wasn’t caught by CI please? I feel like this should have been a warning (that is denied in CI).
There was a problem hiding this comment.
That would be caught with this clippy lint:
https://rust-lang.github.io/rust-clippy/master/index.html#upper_case_acronyms
with additional options in clippy.toml :
avoid-breaking-exported-api = false
upper-case-acronyms-aggressive = true
Unfortunately, This option also warns against SAPRegion and similar (+ TOIEntry, CCDSolver...) . We could suppress it for those specifics.
| @@ -27,8 +29,10 @@ impl CoefficientCombineRule { | |||
|
|
|||
| match effective_rule { | |||
| 0 => (coeff1 + coeff2) / 2.0, | |||
| 1 => coeff1.min(coeff2), | |||
| 1 => coeff1.min(coeff2).abs(), | |||
There was a problem hiding this comment.
Should it be max(0.0) ?
I think there is some use-case for this in godot but that would be worth clarifying by @Ughuuu.
(outside of this PR's scope) @sebcrozet why does this function takes u8 rather than CoefficientCombineRule ?
I don’t think there is a particular reason for this.
